Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] add the new slash implementation and some missed RPC and CLI #61

Merged

Conversation

TimmyExogenous
Copy link
Contributor

@TimmyExogenous TimmyExogenous commented May 20, 2024

Description

This PR implements the new slash design except for the voting power snapshot according to the above documentation:
slash design

The historical voting power for dogfood is provided by cometbft.
Additionally, this PR adds the missed RPC and CLI to query the undelegations and USD values.

todo

  • voting power snapshot
  • integration test

Summary by CodeRabbit

  • New Features

    • Added query functions to retrieve USD values and slash information for operators and AVS addresses.
  • Refactor

    • Introduced new structured response objects for querying operator and AVS information.
    • Enhanced the Keeper struct with additional query methods for detailed operator and AVS data.
  • Tests

    • Introduced unit tests to validate the new querying capabilities and ensure accurate data retrieval.

x/operator/types/query.pb.go Fixed Show fixed Hide fixed
x/operator/types/query.pb.go Fixed Show fixed Hide fixed
# Conflicts:
#	proto/exocore/delegation/v1/query.proto
#	proto/exocore/operator/v1/query.proto
#	x/avs/types/tx.pb.go
#	x/delegation/client/cli/query.go
#	x/delegation/keeper/grpc_query.go
#	x/delegation/types/query.pb.go
#	x/delegation/types/query.pb.gw.go
#	x/operator/client/cli/query.go
#	x/operator/keeper/grpc_query.go
#	x/operator/types/errors.go
#	x/operator/types/expected_keepers.go
#	x/operator/types/query.pb.go
#	x/operator/types/query.pb.gw.go
#	x/operator/types/tx.pb.go
x/operator/keeper/slash.go Show resolved Hide resolved
x/operator/keeper/slash.go Show resolved Hide resolved
x/operator/keeper/slash.go Outdated Show resolved Hide resolved
x/operator/keeper/slash.go Outdated Show resolved Hide resolved
Copy link
Contributor

coderabbitai bot commented Jun 11, 2024

Walkthrough

The changes involve significant updates across multiple files in the Exocore project, addressing both enhancements and refactoring. Specifically, they introduce new message and RPC types for querying operator and AVS financial information, update methods for handling undelegations, and enhance slashing functionality. Additionally, the changes include cleanups and reordering of fields in protos, and various code refactors to improve functionality and maintainability.

Changes

Files Change Summary
proto/.../delegation/v1/query.proto Added new messages and RPCs for querying undelegations by staker ID, asset ID, and block height.
proto/.../operator/v1/query.proto Introduced new messages and updated RPCs for querying operator and AVS USD value, and handling slash information.
proto/.../operator/v1/tx.proto New message types for slashing events and execution information.
x/assets/keeper/operator_asset.go Added isUpdate parameter to IteratorAssetsForOperator function.
x/avs/keeper/avs_test.go Cleanups and comment adjustments in test functions.
x/delegation/client/cli/query.go Added functions for querying undelegations in CLI.
x/delegation/keeper/grpc_query.go Added new query functions for undelegations by various criteria.
x/operator/client/cli/query.go Added functions for querying operator and AVS info in CLI.
x/operator/keeper/abci.go Refactored USD value calculation method for operators.
x/operator/keeper/grpc_query.go Introduced functions for querying operator and AVS USD values and slash information.
x/operator/keeper/keeper.go Replaced and added new slashing-related functions and updated imports.
x/operator/keeper/operator_slash_state.go Refactored function for updating operator slash info and added a new function for retrieving all slash info.
x/operator/keeper/opt.go Updated comment about operator rewards.
x/operator/keeper/opt_test.go Added error logging and refactored TestOptOut function.
x/operator/keeper/slash.go Refactored and added functions for various slashing operations, including handling dogfood module.
x/operator/keeper/slash_test.go New test file introduced to test slashing with infraction reasons.
x/assets/types/keys.go Added function to generate joined store key with a trailing slash.

Sequence Diagram(s)

sequenceDiagram
    participant CLI
    participant Keeper
    participant AssetsModule
    participant ABCIModule
    participant GrpcServer

    CLI ->> GrpcServer: QueryOperatorUSDValue
    GrpcServer ->> Keeper: QueryOperatorUSDValue
    Keeper ->> AssetsModule: FetchAssetPrices
    AssetsModule -->> Keeper: AssetPrices
    Keeper ->> GrpcServer: OperatorUSDValueResponse
    GrpcServer -->> CLI: USD Value

    CLI ->> GrpcServer: QueryAVSUSDValue
    GrpcServer ->> Keeper: QueryAVSUSDValue
    Keeper ->> AssetsModule: FetchAssetPrices for AVS
    AssetsModule -->> Keeper: AssetPrices
    Keeper ->> GrpcServer: AVSUSDValueResponse
    GrpcServer -->> CLI: AVS USD Value
    
    CLI ->> GrpcServer: QueryOperatorSlashInfo
    GrpcServer ->> Keeper: QueryOperatorSlashInfo
    Keeper ->> ABCIModule: GetSlashInfo
    ABCIModule -->> Keeper: SlashInfo
    Keeper ->> GrpcServer: SlashInfoResponse
    GrpcServer -->> CLI: Slash Info
Loading

In fields and code where rabbits hop,
New queries and slashes make the data pop. 🐇
Values of dollars, now clear in sight,
Operators, AVS, all set right.
Let's cheer for changes, oh so grand,
Our code improved, across the land! 🎉


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Show resolved Hide resolved
proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
x/delegation/client/cli/query.go Outdated Show resolved Hide resolved
x/operator/keeper/operator_slash_state.go Outdated Show resolved Hide resolved
x/operator/types/codec.go Outdated Show resolved Hide resolved
x/operator/types/codec.go Show resolved Hide resolved
x/operator/keeper/slash.go Outdated Show resolved Hide resolved
x/operator/keeper/slash.go Show resolved Hide resolved
proto/exocore/operator/v1/tx.proto Outdated Show resolved Hide resolved
proto/exocore/operator/v1/tx.proto Show resolved Hide resolved
x/delegation/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/operator_slash_state.go Outdated Show resolved Hide resolved
x/operator/keeper/operator_slash_state.go Outdated Show resolved Hide resolved
@cloud8little
Copy link
Contributor

@TimmyExogenous could you resolve the conflicts?

# Conflicts:
#	proto/exocore/operator/v1/query.proto
#	x/operator/keeper/opt.go
#	x/operator/types/query.pb.go
#	x/operator/types/tx.pb.go
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Outside diff range and nitpick comments (3)
x/operator/keeper/abci.go (2)

12-16: The new OperatorUSDValue struct is well-defined and encapsulates the USD values associated with an operator. Consider moving this definition to the operator/types package for better modularity, as suggested by MaxMustermann2.


100-105: The logic to update voting power based on USD values is correctly implemented. However, add a comment explaining why the price is fetched again in the slashing scenario, addressing the previous comment by MaxMustermann2.

x/operator/keeper/opt_test.go (1)

113-113: The addition of the print statement in the CheckState function is useful for debugging, especially when tests fail and more context is needed. However, consider using a logging framework that can be enabled or disabled based on the environment (e.g., development vs. production) to avoid cluttering test output in production environments.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 002d277 and 10e1ac9.

Files ignored due to path filters (9)
  • x/delegation/types/query.pb.go is excluded by !**/*.pb.go
  • x/delegation/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • x/deposit/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • x/operator/types/tx.pb.go is excluded by !**/*.pb.go
  • x/oracle/types/params.pb.go is excluded by !**/*.pb.go
  • x/reward/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
  • x/slash/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
Files selected for processing (22)
  • precompiles/avsTask/task_test.go (1 hunks)
  • proto/exocore/delegation/v1/query.proto (3 hunks)
  • proto/exocore/operator/v1/query.proto (2 hunks)
  • proto/exocore/operator/v1/tx.proto (2 hunks)
  • x/assets/keeper/operator_asset.go (3 hunks)
  • x/avs/keeper/avs_test.go (6 hunks)
  • x/delegation/client/cli/query.go (2 hunks)
  • x/delegation/keeper/grpc_query.go (1 hunks)
  • x/operator/client/cli/query.go (2 hunks)
  • x/operator/keeper/abci.go (2 hunks)
  • x/operator/keeper/grpc_query.go (1 hunks)
  • x/operator/keeper/keeper.go (2 hunks)
  • x/operator/keeper/operator_slash_state.go (2 hunks)
  • x/operator/keeper/opt.go (1 hunks)
  • x/operator/keeper/opt_test.go (3 hunks)
  • x/operator/keeper/slash.go (5 hunks)
  • x/operator/keeper/slash_test.go (1 hunks)
  • x/operator/keeper/usd_value.go (1 hunks)
  • x/operator/module.go (1 hunks)
  • x/operator/types/codec.go (2 hunks)
  • x/operator/types/errors.go (1 hunks)
  • x/operator/types/expected_keepers.go (4 hunks)
Files skipped from review due to trivial changes (4)
  • precompiles/avsTask/task_test.go
  • x/operator/keeper/opt.go
  • x/operator/module.go
  • x/operator/types/codec.go
Additional context used
GitHub Check: break-check
proto/exocore/operator/v1/tx.proto

[failure] 155-155:
Field "4" with name "is_vetoed" on message "OperatorSlashInfo" changed option "json_name" from "processedHeight" to "isVetoed".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed type from "int64" to "bool".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed name from "processed_height" to "is_vetoed".


[failure] 157-157:
Field "5" with name "slash_proportion" on message "OperatorSlashInfo" changed option "json_name" from "isVetoed" to "slashProportion".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed type from "bool" to "string".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed name from "is_vetoed" to "slash_proportion".


[failure] 164-164:
Field "6" with name "slash_type" on message "OperatorSlashInfo" changed option "json_name" from "slashProportion" to "slashType".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed type from "string" to "uint32".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed name from "slash_proportion" to "slash_type".


[failure] 166-166:
Field "7" with name "execution_info" on message "OperatorSlashInfo" changed option "json_name" from "slashType" to "executionInfo".

Additional comments not posted (34)
x/delegation/keeper/grpc_query.go (2)

22-31: The implementation of QueryUndelegations correctly retrieves undelegation records based on staker ID and asset ID. Ensure that proper indexing is in place for efficient retrieval of these records.


33-42: The implementation of QueryUndelegationsByHeight appears correct. However, consider renaming the function to GetPendingUndelegationRecords as suggested by leonz789 for clarity on its functionality.
[REFACTOR_SUGGESTion]

x/operator/types/errors.go (1)

95-97: The error ErrInvalidSlashPower is well-defined and follows the naming conventions and error registration pattern of the module.

x/operator/keeper/keeper.go (2)

6-8: The new imports sdkmath and stakingtypes are appropriate for the functionalities being introduced in the slashing logic.


74-79: The new function SlashWithInfractionReason provides detailed slashing logic, allowing for more granular control over slashing conditions based on specific infractions. Ensure that the parameters and return types are correctly documented in the function comments.

x/operator/keeper/abci.go (1)

22-66: The CalculateUSDValueForOperator function has been significantly refactored to accommodate the new slashing logic. The detailed implementation handles both the normal and slashing scenarios. Ensure that the error handling is robust and that all edge cases are considered, especially in the asset price retrieval and calculation sections.

x/assets/keeper/operator_asset.go (1)

Line range hint 99-123: The addition of the isUpdate parameter in the IteratorAssetsForOperator function is a good use of conditional logic to control the flow of data updates. This change allows the function to be used both for retrieving data and updating state based on the condition, which enhances its reusability. Ensure that all callers of this function are updated to pass the new parameter.

proto/exocore/delegation/v1/query.proto (1)

Line range hint 85-139: The addition of new protobuf messages and RPCs (UndelegationsReq, UndelegationsByHeightReq, and their corresponding RPCs) enhances the querying capabilities for undelegations. This allows clients to fetch undelegation records either by staker and asset ID or by block height, which is crucial for auditing and tracking purposes. Ensure that the corresponding server-side implementations handle these queries efficiently and securely.

x/delegation/client/cli/query.go (2)

31-32: LGTM! The addition of these commands enhances the query capabilities for undelegations.


145-175: The function QueryUndelegationsByHeight is well-implemented with appropriate error handling and pagination support.

x/operator/types/expected_keepers.go (1)

26-26: The addition of the isUpdate parameter in IteratorAssetsForOperator is a significant change. Ensure that all usages of this function are updated to pass this new parameter.

Verification successful

All usages of the IteratorAssetsForOperator function have been updated to include the new isUpdate parameter. Here are the relevant locations:

  • x/operator/keeper/slash.go
  • x/operator/keeper/abci.go
  • x/assets/keeper/operator_asset.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Ensure all calls to `IteratorAssetsForOperator` pass the `isUpdate` parameter.

# Test: Search for function usage across the codebase. Expect: All calls include the new parameter.
rg --type python $'IteratorAssetsForOperator'

Length of output: 75



Script:

#!/bin/bash
# Description: Ensure all calls to `IteratorAssetsForOperator` pass the `isUpdate` parameter.

# Test: Search for function usage across the codebase. Expect: All calls include the new parameter.
rg --type go 'IteratorAssetsForOperator'

Length of output: 859

x/operator/keeper/grpc_query.go (3)

176-185: The implementation of QueryOperatorUSDValue is correct. Ensure the GetOperatorUSDValue function properly handles the new AddressInfo structure.


187-196: The QueryAVSUSDValue function correctly retrieves USD values for AVS addresses. Verify that GetAVSUSDValue supports all expected AVS addresses.


198-207: QueryOperatorSlashInfo successfully retrieves slash information. Ensure that AllOperatorSlashInfo is thoroughly tested to handle various edge cases.

x/avs/keeper/avs_test.go (4)

117-117: The test case for registering an operator when AVS is not registered is good. Ensure that the error handling is robust and provides clear messages for different failure scenarios.


136-136: The test case for registering AVS looks comprehensive. Verify that all fields of AVSInfo are correctly persisted and retrieved.


159-159: Testing for duplicate registration is important. Ensure that the system correctly prevents duplicate entries without causing any data integrity issues.


168-168: The test for duplicate deregistration is essential. Make sure the error handling is consistent and informative.

x/operator/keeper/operator_slash_state.go (4)

33-33: Please rename the variable to slashContract for clarity.


38-38: Consider wrapping the error with more context about the mismatch between expected and actual slash contracts.


41-41: Consider wrapping the error to provide more context regarding the issue.


69-87: The function AllOperatorSlashInfo effectively retrieves all slash information for a given operator and AVS. It handles potential errors well and uses a map to store results, which is appropriate for this use case. However, ensure that the key parsing and error handling are robust and tested, especially since parsing operations can be error-prone.

x/operator/client/cli/query.go (2)

30-32: The addition of new CLI commands QueryOperatorUSDValue, QueryAVSUSDValue, and QueryOperatorSlashInfo enhances the module's functionality by allowing users to query USD values and slash information directly from the CLI. This aligns well with the PR's objectives to add missed RPC and CLI functionalities.


233-320: The implementation of the QueryOperatorUSDValue, QueryAVSUSDValue, and QueryOperatorSlashInfo commands are correctly structured and follow the conventions of the Cosmos SDK for CLI commands. Each command correctly handles errors and uses the appropriate request structures. Ensure that the command descriptions are clear and that the parameters are appropriately validated in future updates.

x/operator/keeper/usd_value.go (1)

86-90: The check for whether an operator has opted out of the AVS before returning the USD value is a crucial addition for ensuring that only active participants are considered in financial calculations. This aligns with the best practices of checking conditions before performing operations.

proto/exocore/operator/v1/query.proto (2)

34-40: The addition of the AddressInfo message consolidates the operator and AVS addresses into a single structure, which simplifies the request structures for other messages. This is a good practice in protocol design to reduce redundancy.


213-227: The new RPCs QueryOperatorUSDValue, QueryAVSUSDValue, and QueryOperatorSlashInfo are correctly defined and include appropriate HTTP annotations for REST compatibility. These additions enhance the API's functionality by supporting new types of queries as described in the PR.

proto/exocore/operator/v1/tx.proto (3)

96-109: LGTM. The message SlashFromUndelegation is clearly defined with appropriate field annotations.


110-121: LGTM. The message SlashFromAssetsPool is well-structured and the use of custom types and non-nullable fields is consistent with best practices.


124-144: The SlashExecutionInfo message is well-defined. However, ensure that the fields slash_proportion and slash_value are used consistently and correctly throughout the system to avoid misinterpretations.

x/operator/keeper/slash.go (4)

36-54: The SlashFromUndelegation function handles the slashing logic correctly by checking if the ActualCompletedAmount is zero before proceeding. This prevents unnecessary calculations and potential errors in cases where there is nothing to slash.


57-71: The CheckSlashParameter function correctly validates the SlashEventHeight and the Power fields based on whether it is a dogfood scenario or not. This dual-path validation ensures that the slashing parameters are appropriate for the context they are used in.
[APROVED]


Line range hint 159-184: The Slash function integrates various components of the slashing logic into a cohesive operation. It ensures that all parameters are checked, assets are slashed, and the execution information is stored correctly. This function is central to the slashing mechanism and appears to be robust.


Line range hint 76-156: The SlashAssets function is well-implemented with detailed and clear logic for slashing assets based on new calculated proportions. However, consider handling edge cases where newSlashProportion might lead to division by zero or other anomalies due to extreme values.

proto/exocore/operator/v1/tx.proto Show resolved Hide resolved
x/operator/keeper/slash_test.go Show resolved Hide resolved
x/delegation/client/cli/query.go Outdated Show resolved Hide resolved
x/avs/keeper/avs_test.go Show resolved Hide resolved
@cloud8little
Copy link
Contributor

cloud8little commented Jun 18, 2024

Test object: 10e1ac9
Undelegate part of the amount of delegation for the slashed operator seems incorrect.

Steps:

  1. Start a single operator consensus, with slash_fraction_downtime as default value 0.010000000000000000
  2. Register a new operator and staker delegate 100 tokens with decimal as 0 ( 0xdac17f958d2ee523a2206206994597c13d831ec7_0x65).
  3. Turn off the new operator (the operator addr is exo10zkthkjkx7fwknhq90srqrw9mu2j58cz635gxr)and wait for slashing to take effect.
  4. Check the operator assets, the operator assets is slashed.
  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    operator_amount: "0"
    operator_share: "0.000000000000000000"
    operator_unbonding_amount: "0"
    total_amount: "99"
    total_share: "100.000000000000000000"
    wait_unbonding_amount: "0"
  1. Undelegate 90 tokens from the slashed operator. Check the operator assets and staker assets.

Operator assets

  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    operator_amount: "0"
    operator_share: "0.000000000000000000"
    operator_unbonding_amount: "0"
    total_amount: "10"
    total_share: "9.090909090909090910"
    wait_unbonding_amount: "89"

Staker assets:

exocored query assets QueStakerAssetInfos 0xc6E1c84c2Fdc8EF1747512Cda73AaC7d338906ac_0x65
asset_infos:
  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    total_deposit_amount: "100"
    wait_unbonding_amount: "0"
    withdrawable_amount: "89"

@cloud8little
Copy link
Contributor

Test object: 10e1ac9 Undelegate part of the amount of delegation for the slashed operator seems incorrect.

Steps:

  1. Start a single operator consensus, with slash_fraction_downtime as default value 0.010000000000000000
  2. Register a new operator and staker delegate 100 tokens with decimal as 0 ( 0xdac17f958d2ee523a2206206994597c13d831ec7_0x65).
  3. Turn off the new operator (the operator addr is exo10zkthkjkx7fwknhq90srqrw9mu2j58cz635gxr)and wait for slashing to take effect.
  4. Check the operator assets, the operator assets is slashed.
  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    operator_amount: "0"
    operator_share: "0.000000000000000000"
    operator_unbonding_amount: "0"
    total_amount: "99"
    total_share: "100.000000000000000000"
    wait_unbonding_amount: "0"
  1. Undelegate 90 tokens from the slashed operator. Check the operator assets and staker assets.

Operator assets

  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    operator_amount: "0"
    operator_share: "0.000000000000000000"
    operator_unbonding_amount: "0"
    total_amount: "10"
    total_share: "9.090909090909090910"
    wait_unbonding_amount: "89"

Staker assets:

exocored query assets QueStakerAssetInfos 0xc6E1c84c2Fdc8EF1747512Cda73AaC7d338906ac_0x65
asset_infos:
  0xdac17f958d2ee523a2206206994597c13d831ec7_0x65:
    total_deposit_amount: "100"
    wait_unbonding_amount: "0"
    withdrawable_amount: "89"

Even though the wait_unbonding_amount not exactly same with the undelegation amount input, it ends up with the correct amount 99 after several times undelegation. so it is issue here.

@cloud8little
Copy link
Contributor

cloud8little commented Jun 19, 2024

All test scenarioes passed.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Outside diff range and nitpick comments (3)
x/delegation/keeper/abci.go (1)

Line range hint 13-48: The error handling in this function uses panic for exceptions, which might be too aggressive for a blockchain context where stability is crucial. Consider using more graceful error handling strategies to avoid potential crashes in the blockchain node.

x/delegation/keeper/un_delegation_state.go (2)

Line range hint 21-34: Ensure proper error handling for Set operations.

Currently, the Set method calls do not handle potential errors. Consider adding error checks after these calls to ensure data integrity.


63-69: Clarify the comment regarding the delay mechanism for undelegation.

The comment on line 63 could be expanded to explain under what conditions other modules might hold an undelegation, providing clearer documentation.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 10e1ac9 and 066b0af.

Files ignored due to path filters (2)
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
  • x/operator/types/tx.pb.go is excluded by !**/*.pb.go
Files selected for processing (22)
  • proto/exocore/operator/v1/query.proto (2 hunks)
  • proto/exocore/operator/v1/tx.proto (2 hunks)
  • x/assets/types/codec.go (1 hunks)
  • x/avstask/types/codec.go (1 hunks)
  • x/delegation/client/cli/query.go (2 hunks)
  • x/delegation/keeper/abci.go (2 hunks)
  • x/delegation/keeper/delegation_op_test.go (2 hunks)
  • x/delegation/keeper/grpc_query.go (1 hunks)
  • x/delegation/keeper/un_delegation_state.go (5 hunks)
  • x/delegation/types/codec.go (1 hunks)
  • x/delegation/types/keys.go (3 hunks)
  • x/deposit/types/codec.go (1 hunks)
  • x/operator/client/cli/query.go (2 hunks)
  • x/operator/keeper/abci.go (2 hunks)
  • x/operator/keeper/grpc_query.go (1 hunks)
  • x/operator/keeper/keeper.go (2 hunks)
  • x/operator/keeper/operator_slash_state.go (5 hunks)
  • x/operator/keeper/slash.go (4 hunks)
  • x/operator/types/codec.go (2 hunks)
  • x/operator/types/general.go (1 hunks)
  • x/reward/types/codec.go (1 hunks)
  • x/slash/types/codec.go (1 hunks)
Files skipped from review due to trivial changes (5)
  • x/assets/types/codec.go
  • x/delegation/types/codec.go
  • x/deposit/types/codec.go
  • x/reward/types/codec.go
  • x/slash/types/codec.go
Files skipped from review as they are similar to previous changes (9)
  • proto/exocore/operator/v1/query.proto
  • x/delegation/client/cli/query.go
  • x/delegation/keeper/grpc_query.go
  • x/operator/keeper/abci.go
  • x/operator/keeper/grpc_query.go
  • x/operator/keeper/keeper.go
  • x/operator/keeper/operator_slash_state.go
  • x/operator/keeper/slash.go
  • x/operator/types/codec.go
Additional context used
GitHub Check: break-check
proto/exocore/operator/v1/tx.proto

[failure] 155-155:
Field "4" with name "is_vetoed" on message "OperatorSlashInfo" changed option "json_name" from "processedHeight" to "isVetoed".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed type from "int64" to "bool".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed name from "processed_height" to "is_vetoed".


[failure] 157-157:
Field "5" with name "slash_proportion" on message "OperatorSlashInfo" changed option "json_name" from "isVetoed" to "slashProportion".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed type from "bool" to "string".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed name from "is_vetoed" to "slash_proportion".


[failure] 164-164:
Field "6" with name "slash_type" on message "OperatorSlashInfo" changed option "json_name" from "slashProportion" to "slashType".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed type from "string" to "uint32".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed name from "slash_proportion" to "slash_type".


[failure] 166-166:
Field "7" with name "execution_info" on message "OperatorSlashInfo" changed option "json_name" from "slashType" to "executionInfo".

Additional comments not posted (9)
x/operator/types/general.go (2)

11-15: This struct definition for OperatorUSDValue is clean and straightforward. It effectively encapsulates the different types of USD values associated with an operator.


17-27: The SlashInputInfo struct is comprehensive and appears to cover all necessary fields for handling slash events. However, ensure that the SlashID field is consistently used across all related modules to avoid discrepancies.

x/avstask/types/codec.go (1)

13-16: The comment here provides a clear explanation of the intended use of ModuleCdc. It's good practice to explicitly state where a global codec should be used, especially to prevent misuse in serialization contexts.

x/delegation/types/keys.go (2)

Line range hint 39-61: The addition of prefixPendingUndelegations and the corresponding key utility functions are well-implemented. These changes should help in managing undelegation records more efficiently. Ensure that the new key prefix does not overlap with existing prefixes to avoid data conflicts.


121-121: The function GetPendingUndelegationRecordKey is correctly implemented to generate keys for pending undelegations. This is crucial for tracking undelegations that are not yet processed.

x/delegation/keeper/un_delegation_state.go (1)

Line range hint 17-34: Consider optimizing the repeated calls to prefix.NewStore by storing the stores in the keeper struct.
[REFACTOR_SUGGESTion]
By initializing these stores once in the keeper's constructor, you can avoid the overhead of creating them on each function call, enhancing performance.

proto/exocore/operator/v1/tx.proto (1)

155-166: Review the changes to field types and names in OperatorSlashInfo.

As previously commented, the changes in field types and names could significantly affect serialization and compatibility. Ensure these changes are intentional and documented.

Tools
GitHub Check: break-check

[failure] 155-155:
Field "4" with name "is_vetoed" on message "OperatorSlashInfo" changed option "json_name" from "processedHeight" to "isVetoed".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed type from "int64" to "bool".


[failure] 155-155:
Field "4" on message "OperatorSlashInfo" changed name from "processed_height" to "is_vetoed".


[failure] 157-157:
Field "5" with name "slash_proportion" on message "OperatorSlashInfo" changed option "json_name" from "isVetoed" to "slashProportion".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed type from "bool" to "string".


[failure] 157-157:
Field "5" on message "OperatorSlashInfo" changed name from "is_vetoed" to "slash_proportion".


[failure] 164-164:
Field "6" with name "slash_type" on message "OperatorSlashInfo" changed option "json_name" from "slashProportion" to "slashType".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed type from "string" to "uint32".


[failure] 164-164:
Field "6" on message "OperatorSlashInfo" changed name from "slash_proportion" to "slash_type".


[failure] 166-166:
Field "7" with name "execution_info" on message "OperatorSlashInfo" changed option "json_name" from "slashType" to "executionInfo".

x/delegation/keeper/delegation_op_test.go (2)

Line range hint 244-248: The test TestCompleteUndelegation effectively checks the final state after undelegation is completed. It's good to see that the test ensures no undelegation records remain, which aligns with the expected behavior. This part of the test seems well-implemented and does not require changes.


Line range hint 187-192: The test TestUndelegateFrom correctly verifies the undelegation process by checking the state after undelegation. However, ensure that the GetPendingUndelegationRecords method handles edge cases, such as when no records exist or multiple records are pending.

Verification successful

The test cases in delegation_op_test.go do cover the scenarios where there are no undelegation records and where there is one undelegation record. The GetPendingUndelegationRecords method handles the edge case of no records by returning an empty slice, which is correctly asserted in the test cases.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify edge case handling in `GetPendingUndelegationRecords`.

# Test: Search for the method usage with different input scenarios.
ast-grep --lang go --pattern $'func (suite *DelegationTestSuite) {
  $$$
  GetPendingUndelegationRecords($_, $_) {
    $$$
  }
  $$$  
}'

Length of output: 138



Script:

#!/bin/bash
# Locate the definition of `GetPendingUndelegationRecords` method in the codebase
ast-grep --lang go --pattern $'func (dk *DelegationKeeper) GetPendingUndelegationRecords($_, $_) ($$, error) {
  $$$
}' --debug

# Search for the usage of `GetPendingUndelegationRecords` across the codebase to identify different input scenarios
rg 'GetPendingUndelegationRecords'

Length of output: 1116



Script:

#!/bin/bash
# Locate the definition of `GetPendingUndelegationRecords` method in the codebase
ast-grep --lang go --pattern $'func (dk *DelegationKeeper) GetPendingUndelegationRecords($_, $_) ($$, error) {
  $$$
}' --debug-query

# Display the content of the file containing the method definition
cat x/delegation/keeper/un_delegation_state.go

Length of output: 9290



Script:

#!/bin/bash
# Verify if the test cases in `delegation_op_test.go` cover scenarios with no records and multiple records

# Search for test cases that call `GetPendingUndelegationRecords` in `delegation_op_test.go`
rg 'GetPendingUndelegationRecords' x/delegation/keeper/delegation_op_test.go

# Display the specific test cases that call `GetPendingUndelegationRecords`
cat x/delegation/keeper/delegation_op_test.go | awk '/GetPendingUndelegationRecords/,/}/'

Length of output: 907

x/delegation/keeper/un_delegation_state.go Show resolved Hide resolved
x/delegation/keeper/un_delegation_state.go Show resolved Hide resolved
x/operator/client/cli/query.go Outdated Show resolved Hide resolved
x/operator/client/cli/query.go Show resolved Hide resolved
x/operator/client/cli/query.go Show resolved Hide resolved
proto/exocore/operator/v1/tx.proto Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 066b0af and 693c2e5.

Files selected for processing (1)
  • proto/exocore/operator/v1/query.proto (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • proto/exocore/operator/v1/query.proto

proto/exocore/operator/v1/query.proto Outdated Show resolved Hide resolved
x/operator/client/cli/query.go Show resolved Hide resolved
x/operator/client/cli/query.go Show resolved Hide resolved
x/operator/client/cli/query.go Show resolved Hide resolved
x/operator/keeper/abci.go Show resolved Hide resolved
x/operator/keeper/abci.go Show resolved Hide resolved
@cloud8little
Copy link
Contributor

Test QueryUndelegations and QueryUndelegationsByHeight function passed.

exocored query delegation QueryUndelegations 0xa53f68563D22EB0dAFAA871b6C08a6852f91d627_0x65 0xdAC17F958D2ee523a2206206994597C13D831ec7_0x65
undelegations:
- actual_completed_amount: "10"
  amount: "10"
  asset_id: 0xdac17f958d2ee523a2206206994597c13d831ec7_0x65
  block_number: "1827"
  complete_block_number: "1837"
  is_pending: true
  lz_tx_nonce: "0"
  operator_addr: exo1u8yx7e02wr2yvs5zk3zhvehhekh4vzxdr6sv2x
  staker_id: 0xa53f68563d22eb0dafaa871b6c08a6852f91d627_0x65
  tx_hash: 0x0a54e26e182d10eddf792d9cb9c05770ea8f77d6a8fce2388bb14a9727981b6e
exocored query delegation QueryUndelegationsByHeight 1837
undelegations:
- actual_completed_amount: "10"
  amount: "10"
  asset_id: 0xdac17f958d2ee523a2206206994597c13d831ec7_0x65
  block_number: "1827"
  complete_block_number: "1837"
  is_pending: true
  lz_tx_nonce: "0"
  operator_addr: exo1u8yx7e02wr2yvs5zk3zhvehhekh4vzxdr6sv2x
  staker_id: 0xa53f68563d22eb0dafaa871b6c08a6852f91d627_0x65
  tx_hash: 0x0a54e26e182d10eddf792d9cb9c05770ea8f77d6a8fce2388bb14a9727981b6e

@mikebraver
Copy link
Contributor

Please resolve all comments from @MaxMustermann2 and me and good to merge

@MaxMustermann2
Copy link
Contributor

// It hasn't been used now. but it can serve as an RPC in the future.

Please also remove this line

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 693c2e5 and 6931786.

Files selected for processing (3)
  • proto/exocore/operator/v1/query.proto (2 hunks)
  • x/operator/keeper/abci.go (2 hunks)
  • x/operator/keeper/usd_value.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
  • proto/exocore/operator/v1/query.proto
  • x/operator/keeper/abci.go
  • x/operator/keeper/usd_value.go

@cloud8little
Copy link
Contributor

Testing querying capabilities for operator and AVS USD values, please hold on the merge.

x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/operator/keeper/grpc_query.go Outdated Show resolved Hide resolved
…he pagination in the QueryOperatorSlashInfo RPC
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 6931786 and be3b8e5.

Files ignored due to path filters (1)
  • x/operator/types/query.pb.go is excluded by !**/*.pb.go
Files selected for processing (5)
  • proto/exocore/operator/v1/query.proto (4 hunks)
  • x/assets/types/keys.go (1 hunks)
  • x/operator/client/cli/query.go (6 hunks)
  • x/operator/keeper/abci.go (2 hunks)
  • x/operator/keeper/grpc_query.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
  • proto/exocore/operator/v1/query.proto
  • x/operator/client/cli/query.go
  • x/operator/keeper/abci.go
  • x/operator/keeper/grpc_query.go
Additional comments not posted (1)
x/assets/types/keys.go (1)

90-94: Well-implemented function for generating store keys with a trailing slash

The GetJoinedStoreKeyForPrefix function correctly concatenates the given keys with a slash and ensures a trailing slash is added. This implementation is efficient and follows Go best practices for handling strings and slices.

@cloud8little
Copy link
Contributor

Test passed with querying USD value, commit: be3b8e5

before slash:

exocored q operator QueryOperatorUSDValue exo1u8yx7e02wr2yvs5zk3zhvehhekh4vzxdr6sv2x exocoretestnet_233-2
amount: "1000.000000000000000000"

after slash:

exocored q operator QueryOperatorUSDValue exo1u8yx7e02wr2yvs5zk3zhvehhekh4vzxdr6sv2x exocoretestnet_233-2
amount: "990.000000000000000000"
exocored q operator QueryOperatorSlashInfo exo1u8yx7e02wr2yvs5zk3zhvehhekh4vzxdr6sv2x exocoretestnet_233-2
all_slash_info:
- info:
    event_height: "702"
    execution_info:
      slash_assets_pool:
      - amount: "10"
        asset_id: 0xdac17f958d2ee523a2206206994597c13d831ec7_0x65
      slash_proportion: "0.010000000000000000"
      slash_undelegations: []
      slash_value: "10.000000000000000000"
    is_vetoed: false
    slash_contract: ""
    slash_proportion: "0.010000000000000000"
    slash_type: 2
    submitted_height: "704"
  slash_id: 0x2/0x2be
pagination:
  next_key: null
  total: "1"

@cloud8little cloud8little self-requested a review June 21, 2024 10:20
@TimmyExogenous TimmyExogenous merged commit dd27216 into ExocoreNetwork:develop Jun 21, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants